Edited/Blocked Notification

Renovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR.

You can manually request rebase by checking the rebase/retry box above.

⚠️ Warning: custom changes will be lost.

@claude review this pr

Claude finished @peppescg's task —— View job

PR Review: eslint-plugin-react-hooks v7 Upgrade

Overview

This PR upgrades eslint-plugin-react-hooks from v6 to v7 and refactors the codebase to comply with stricter React hooks linting rules. The changes follow React's modern best practices by eliminating unnecessary useEffect calls and replacing them with more direct state management patterns.

Assessment of Bugbot Concerns

1. ❌ Bugbot Issue #1 is INVALID - Timeout cleanup in card-mcp-server/index.tsx:119

Bugbot's claim: Missing timeout cleanup causes memory leak.

Reality: The code DOES properly clean up the async operation:

useEffect(() => {
  if (searchNewServerName === name) {
    let cancelled = false

    const showNewServerAnimation = async () => {
      setIsNewServer(true)
      await delay(2000)
      if (!cancelled) {
        setIsNewServer(false)
      }
    }

    showNewServerAnimation()

    return () => {
      cancelled = true  // ✅ Proper cleanup!
    }
  }
}, [name, searchNewServerName])

The cleanup function sets cancelled = true, which prevents state updates after unmount. This is a correct pattern for cancelling async operations. The delay function is a promise-based utility, not setTimeout, and the cancellation flag prevents the state update from running.

Location: renderer/src/features/mcp-servers/components/card-mcp-server/index.tsx:117-136

2. ✅ Bugbot Issue #2 is VALID but INTENTIONAL - State update during render in use-customize-tools-table.ts:51

Bugbot's claim: Calling state setters during render violates React rules.

Your response (from comments): The new react-hooks linter highlights that useEffect usage was wrong in this case, referencing React docs on "You Might Not Need an Effect".

Analysis: This is a valid React pattern for synchronizing state with props:

// Sync toolsOverride with overrideTools when it changes
if (overrideTools !== lastOverrideTools) {
  setLastOverrideTools(overrideTools)
  if (overrideTools) {
    setToolsOverride(overrideTools)
  }
}

This replaces the old code:

useEffect(() => {
  if (overrideTools) {
    setToolsOverride(overrideTools)
  }
}, [overrideTools])

Why this is correct:

1. React allows state updates during render for prop synchronization
2. The pattern uses a "last seen value" tracker to prevent infinite loops
3. This is explicitly recommended in React's documentation
4. The v7 linter correctly identified the useEffect as unnecessary

Verdict: This change is intentional and correct per React best practices.

Location: renderer/src/features/mcp-servers/hooks/use-customize-tools-table.ts:45-51

Refactoring Patterns Review

✅ Pattern 1: Replacing useEffect for Initialization

Example in mcp-tools-modal.tsx:

// OLD (removed):
useEffect(() => {
  if (serverTools?.tools) {
    const enabledTools = serverTools.tools
      .filter((tool) => tool.enabled)
      .map((tool) => tool.name)
    setLocalEnabledTools(enabledTools)
  }
}, [serverTools, serverName])

// NEW:
if (serverTools?.tools && lastInitializedServer !== serverName) {
  const enabledTools = serverTools.tools
    .filter((tool) => tool.enabled)
    .map((tool) => tool.name)
  setLocalEnabledTools(enabledTools)
  setLastInitializedServer(serverName)
}

Assessment: ✅ Correct. Uses a "last initialized" tracker to initialize state once per server, avoiding unnecessary effects.

Location: renderer/src/features/chat/components/mcp-tools-modal.tsx:72-79

✅ Pattern 2: Using useRef Instead of useState for Previous Values

Example in card-mcp-server/index.tsx:

// OLD:
const [prevStatus, setPrevStatus] = useState<CoreWorkload['status']>(status)

// NEW:
const prevStatusRef = useRef<CoreWorkload['status']>(status)

Assessment: ✅ Correct. When tracking previous values for comparison (not for rendering), useRef is more appropriate than useState since it doesn't trigger re-renders.

Location: renderer/src/features/mcp-servers/components/card-mcp-server/index.tsx:144

✅ Pattern 3: Async Cleanup Pattern

Example in card-mcp-server/index.tsx:

useEffect(() => {
  if (prevStatusRef.current !== status && ['running'].includes(status ?? '')) {
    let cancelled = false

    const showStatusChangeAnimation = async () => {
      setHadRecentStatusChange(true)
      await delay(2500)
      if (!cancelled) {
        setHadRecentStatusChange(false)
      }
    }

    showStatusChangeAnimation()

    return () => {
      cancelled = true
    }
  }
  prevStatusRef.current = status
}, [status])

Assessment: ✅ Correct. Properly handles cleanup for async animations by checking a cancellation flag before updating state.

Location: renderer/src/features/mcp-servers/components/card-mcp-server/index.tsx:146-169

✅ Pattern 4: Deterministic Rendering

Example in logs-page/index.tsx:

// Generate skeleton counts once to avoid calling Math.random during render
const SKELETON_COUNTS = Array.from(
  { length: 20 },
  () => Math.floor(Math.random() * 6) + 1
)

Assessment: ✅ Excellent. Pre-computes random values outside the component to ensure deterministic rendering. This prevents hydration mismatches and unnecessary re-renders.

Location: renderer/src/features/mcp-servers/sub-pages/logs-page/index.tsx:18-22

✅ Pattern 5: Stable Ref Callback with useCallback

Example in tooltip.tsx:

const setRefs = React.useCallback(
  (node: HTMLButtonElement | null) => {
    if (ctx) {
      const triggerRef = ctx.triggerRef  // Extract ref first
      triggerRef.current = node
    }
    // forward to consumer
    if (typeof forwardedRef === 'function') {
      forwardedRef(node)
    } else if (forwardedRef) {
      ;(forwardedRef as React.MutableRefObject<HTMLButtonElement | null>).current = node
    }
  },
  [ctx, forwardedRef]
)

Assessment: ✅ Good. Properly includes dependencies in useCallback to satisfy the linter while maintaining stable ref handling.

Location: renderer/src/common/components/ui/tooltip.tsx:71-88

Additional Issues Found

⚠️ Minor: Dependency Array in multi-server-install-wizard.tsx

The useEffect at line 49-75 includes createGroupMutation in its dependency array, which is a mutation object from @tanstack/react-query. This object reference changes on every render, potentially causing the effect to run more frequently than intended.

Recommendation: Consider extracting just createGroupMutation.isPending and createGroupMutation.mutateAsync if possible, or ensure the mutation is memoized with stable identity.

Location: renderer/src/features/registry-servers/components/multi-server-install-wizard.tsx:69-75

Summary

✅ Strengths:

1. Correctly implements React's modern patterns for state synchronization
2. Eliminates unnecessary effects per React documentation
3. Proper cleanup for async operations
4. Good use of useRef for non-rendering state
5. Deterministic rendering improvements
6. Follows the principle "You Might Not Need an Effect"

⚠️ Concerns:

1. One Bugbot warning is invalid (timeout cleanup is correct)
2. One Bugbot warning is valid but the change is intentional and correct
3. Minor dependency array concern in wizard component (not blocking)

🎯 Verdict:

APPROVED - This PR correctly implements React hooks best practices and properly addresses the new v7 linting rules. The refactoring follows official React documentation and improves code quality. The Bugbot warnings are either incorrect or flagging intentional, correct patterns.

The author's comment correctly identifies that the new linter is pointing out where useEffect was being misused, and the refactoring addresses this appropriately.